perf(uniq): Improve performance of uniq function
#40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@ho991217 is attempting to deploy a commit to the Toss Team on Vercel. A member of the Team first needs to authorize it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 46 46
Lines 217 211 -6
Branches 20 19 -1
=========================================
- Hits 217 211 -6 |
raon0211
reviewed
Jun 13, 2024
raon0211
changed the title
uniq functionuniq function
raon0211
approved these changes
Jun 13, 2024
There was a problem hiding this comment.
Thanks! It was originally implemented by using Set, but if the array is small, using arrays were more performant.
However I guess this will not make a big difference with small arrays, and will make a huge difference with big arrays. Thanks for your contribution again!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What have been changed
Previous code used
for ... of ...statement to check whether an item included in result array. I've changed it to return unique values withSet.Improvement
for ... of ...isO(N).Array.prototype.includesdoes linear search, so it isO(N)considering the worst case scenario. So the code before had time complexity ofO(N^2).new Set()isO(N),Array.from()is alsoO(N), so total time complexity becomesO(N).100,00025,00075,000existing code: 542.303459 msupdated code: 3.931583 msPossible drawbacks
Array.prototype.includeshave out with ES7, on the other handSethave done with ES6. So updated method also supports older version without polyfill.Setalso keeps its insertion order.uniqfunction createsSetobject every time when function is called. There is chance that slight increase in memory usage. But I guess it does not make huge difference from makingresultarray every time.Added test cases
I've added test cases related to
uniqfunction updates which assures its original functions.Considering performance is primary factor of
es-toolkitlib, this pr should be fair enough.Let me know if other drawback does this pr have.